Skip to content

style(clang-tidy):Use hh extension for cpp header files #1645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

bavulapati
Copy link
Contributor

Clang tidy assumes the .h files as C headers.

And that makes it not find standard library files like , <intializer_list>, <string_view>, etc.,
Somehow my VSCode also recognises it as C header.

make lint
cmake --build ./build --config Debug --target clang_tidy
gmake[1]: Entering directory '/Users/balakrishna/repos/sourcemeta/core/build'
-- Enabling SIMD NEON
-- Google Benchmark version: v1.8.5, normalized to 1.8.5
-- Enabling additional flags: -DINCLUDE_DIRECTORIES=/Users/balakrishna/repos/sourcemeta/core/vendor/googlebenchmark/include
-- Performing Test HAVE_THREAD_SAFETY_ATTRIBUTES -- success
-- Performing Test HAVE_STD_REGEX -- success
-- Performing Test HAVE_GNU_POSIX_REGEX -- failed to compile
-- Performing Test HAVE_POSIX_REGEX -- success
-- Performing Test HAVE_STEADY_CLOCK -- success
-- Performing Test HAVE_PTHREAD_AFFINITY -- failed to compile
-- Configuring done (0.3s)
-- Generating done (0.3s)
-- Build files have been written to: /Users/balakrishna/repos/sourcemeta/core/build
[100%] Analyzing sources using ClangTidy
102 warnings and 1 error generated.
Error while processing /Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h.
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h:8:1: error: included header json_hash.h is not used directly [misc-include-cleaner,-warnings-as-errors]
    8 | #include <sourcemeta/core/json_hash.h>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    9 | #include <sourcemeta/core/json_value.h>
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h:48:6: error: variable 'parse_json' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables,-warnings-as-errors]
   48 | auto parse_json(std::basic_istream<JSON::Char, JSON::CharTraits> &stream,
      |      ^
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h:88:6: error: variable 'parse_json' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables,-warnings-as-errors]
   88 | auto parse_json(std::basic_istream<JSON::Char, JSON::CharTraits> &stream,
      |      ^
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json_hash.h:4:10: error: 'cassert' file not found [clang-diagnostic-error]
    4 | #include <cassert> // assert
      |          ^~~~~~~~~
Suppressed 99 warnings (99 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
3 warnings treated as errors
gmake[4]: *** [CMakeFiles/clang_tidy.dir/build.make:71: CMakeFiles/clang_tidy] Error 1
gmake[3]: *** [CMakeFiles/Makefile2:825: CMakeFiles/clang_tidy.dir/all] Error 2
gmake[2]: *** [CMakeFiles/Makefile2:832: CMakeFiles/clang_tidy.dir/rule] Error 2
gmake[1]: *** [Makefile:257: clang_tidy] Error 2
gmake[1]: Leaving directory '/Users/balakrishna/repos/sourcemeta/core/build'
make: *** [lint] Error 2
Screenshot 2025-05-14 at 3 25 22 PM

Clang tidy assumes the .h files as C headers.
And that makes it not find standard library files like <cassert>, <intializer_list>, <string_view>, etc.,
Somehow my VSCode also recognises it as C header.

```
make lint
cmake --build ./build --config Debug --target clang_tidy
gmake[1]: Entering directory '/Users/balakrishna/repos/sourcemeta/core/build'
-- Enabling SIMD NEON
-- Google Benchmark version: v1.8.5, normalized to 1.8.5
-- Enabling additional flags: -DINCLUDE_DIRECTORIES=/Users/balakrishna/repos/sourcemeta/core/vendor/googlebenchmark/include
-- Performing Test HAVE_THREAD_SAFETY_ATTRIBUTES -- success
-- Performing Test HAVE_STD_REGEX -- success
-- Performing Test HAVE_GNU_POSIX_REGEX -- failed to compile
-- Performing Test HAVE_POSIX_REGEX -- success
-- Performing Test HAVE_STEADY_CLOCK -- success
-- Performing Test HAVE_PTHREAD_AFFINITY -- failed to compile
-- Configuring done (0.3s)
-- Generating done (0.3s)
-- Build files have been written to: /Users/balakrishna/repos/sourcemeta/core/build
[100%] Analyzing sources using ClangTidy
102 warnings and 1 error generated.
Error while processing /Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h.
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h:8:1: error: included header json_hash.h is not used directly [misc-include-cleaner,-warnings-as-errors]
    8 | #include <sourcemeta/core/json_hash.h>
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    9 | #include <sourcemeta/core/json_value.h>
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h:48:6: error: variable 'parse_json' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables,-warnings-as-errors]
   48 | auto parse_json(std::basic_istream<JSON::Char, JSON::CharTraits> &stream,
      |      ^
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json.h:88:6: error: variable 'parse_json' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables,-warnings-as-errors]
   88 | auto parse_json(std::basic_istream<JSON::Char, JSON::CharTraits> &stream,
      |      ^
/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json_hash.h:4:10: error: 'cassert' file not found [clang-diagnostic-error]
    4 | #include <cassert> // assert
      |          ^~~~~~~~~
Suppressed 99 warnings (99 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
3 warnings treated as errors
gmake[4]: *** [CMakeFiles/clang_tidy.dir/build.make:71: CMakeFiles/clang_tidy] Error 1
gmake[3]: *** [CMakeFiles/Makefile2:825: CMakeFiles/clang_tidy.dir/all] Error 2
gmake[2]: *** [CMakeFiles/Makefile2:832: CMakeFiles/clang_tidy.dir/rule] Error 2
gmake[1]: *** [Makefile:257: clang_tidy] Error 2
gmake[1]: Leaving directory '/Users/balakrishna/repos/sourcemeta/core/build'
make: *** [lint] Error 2
```

Signed-off-by: Balakrishna Avulapati <[email protected]>
@bavulapati
Copy link
Contributor Author

bavulapati commented May 14, 2025

@jviotti I want to know your thoughts before changing the extension of rest of the headers

@bavulapati
Copy link
Contributor Author

Refs sourcemeta/blaze#429

@bavulapati
Copy link
Contributor Author

picked up the extension hh since we are using cc for implementation files

@jviotti
Copy link
Member

jviotti commented May 14, 2025

That's weird. .h is pretty common in C++. There must be a way to tell ClangTidy that the project is C++, and not have it look at the extension. That would be much preferred over renaming the headers to .hh

@jviotti
Copy link
Member

jviotti commented May 24, 2025

I'm also looking into this problem. So the main symptom of .h detected as C headers is this, right?

/Users/balakrishna/repos/sourcemeta/core/src/core/json/include/sourcemeta/core/json_hash.h:4:10: error: 'cassert' file not found [clang-diagnostic-error]
    4 | #include <cassert> // assert
      |          ^~~~~~~~~

@jviotti
Copy link
Member

jviotti commented May 24, 2025

Actually, I've been researching this more. So the way ClangTidy works is based on compile_commands.json. Which means that if you try to lint a header directly, ClangTidy gets confused as it won't find a compilation unit for the header (rightfully so), and that's the root of the problem. The suggestion out there is to never lint headers directly, but .cc files (that will lint the headers as a result through the includes)

jviotti added a commit that referenced this pull request May 24, 2025
See: #1645
Signed-off-by: Juan Cruz Viotti <[email protected]>
@jviotti
Copy link
Member

jviotti commented May 24, 2025

OK, see this: #1686. I'll close this issue as I think we won't need it after all.

In any case, thanks for pushing for ClangTidy. We are fixing and learning a lot of little things :)

@jviotti jviotti closed this May 24, 2025
jviotti added a commit that referenced this pull request May 24, 2025
@bavulapati
Copy link
Contributor Author

Actually, I've been researching this more. So the way ClangTidy works is based on compile_commands.json. Which means that if you try to lint a header directly, ClangTidy gets confused as it won't find a compilation unit for the header (rightfully so), and that's the root of the problem. The suggestion out there is to never lint headers directly, but .cc files (that will lint the headers as a result through the includes)

I also read about 'compile_commands.json' but not this suggestion. Nice to know.

@bavulapati bavulapati deleted the use-hh-extension-for-cpp-header-files branch May 24, 2025 05:41
@bavulapati
Copy link
Contributor Author

OK, see this: #1686. I'll close this issue as I think we won't need it after all.

In any case, thanks for pushing for ClangTidy. We are fixing and learning a lot of little things :)

Yeah. Nice learnings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants